Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add the sha of the the current commit to the watermark as the fragment of the watermark image url. #1309

Merged
merged 4 commits into from
Jan 13, 2023
Merged

Add the sha of the the current commit to the watermark as the fragment of the watermark image url. #1309

merged 4 commits into from
Jan 13, 2023
+194 −29

Conversation

tasdomas
Copy link
Contributor

This PR also updates the way updatable comments are determined - the matcher should ignore the sha in
the watermark.

This PR supercedes #1304.

@tasdomas
Watermark refactoring.
fd9e869 
Watermarks will include the sha of the current commit as the fragment part of the watermark image url (https://cml.dev/watermark.png#{SHA}).
The updatable comment lookup has also been refactored to match watermarks with different commit sha's.
@tasdomas tasdomas temporarily deployed to internal January 11, 2023 14:01 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas requested a review from a team January 11, 2023 15:53
@tasdomas tasdomas self-assigned this Jan 11, 2023
@tasdomas tasdomas added bug Something isn't working cml-comment Subcommand labels Jan 11, 2023
DavidGOrtega
DavidGOrtega suggested changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just only a little observation that might be even incorrect

src/cml.js Outdated Show resolved Hide resolved
@tasdomas tasdomas temporarily deployed to internal January 12, 2023 15:11 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas temporarily deployed to internal January 12, 2023 16:29 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas temporarily deployed to internal January 12, 2023 17:03 — with GitHub Actions Inactive
@tasdomas tasdomas temporarily deployed to internal January 12, 2023 17:09 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas temporarily deployed to internal January 12, 2023 20:05 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

dacbd
dacbd approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@dacbd dacbd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it more than mine.
iterative/cml-playground#294

@tasdomas tasdomas temporarily deployed to internal January 13, 2023 09:26 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@tasdomas tasdomas requested a review from DavidGOrtega January 13, 2023 15:20
DavidGOrtega
DavidGOrtega approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🚀

@tasdomas tasdomas merged commit 2186356 into master Jan 13, 2023
@tasdomas tasdomas deleted the d018-watermark-sha branch January 13, 2023 16:16
@dacbd dacbd mentioned this pull request Feb 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dacbd dacbd dacbd approved these changes

@DavidGOrtega DavidGOrtega DavidGOrtega approved these changes

Assignees

@tasdomas tasdomas

Labels
bug Something isn't working cml-comment Subcommand
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tasdomas @DavidGOrtega @dacbd